Repository navigation
Conversation
…ng (modelcontextprotocol#1308) tools/call crashed for any outputSchema that is not an object schema (.optional(), .nullable(), .nullish(), z.union(...)): normalizeObjectSchema returns undefined for non-object roots, and validateToolOutput passed that undefined straight to safeParseAsync ("Cannot read properties of undefined (reading '_zod')"). The same registration also silently omitted the schema from tools/list, which disabled client-side success validation for the tool (clients only build validators from the listing). - validateToolOutput now falls back to parsing the schema directly when normalization fails, mirroring the input-validation path - registration warns when an output schema cannot be advertised, so the listing omission is visible instead of silent. It cannot be converted: the wire schema requires type "object" at the outputSchema root per the MCP specification, so an anyOf root would make spec-valid clients reject the listing itself - tests cover all four root shapes over the zod v3/v4 matrix, pin the listing omission as documented behaviour, and assert the warning
🦋 Changeset detectedLatest commit: 87d9448 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
| // Try to normalize to object schema first (for raw shapes and object schemas) | ||
| // If that fails, use the schema directly (for union/intersection/etc), mirroring | ||
| // the input-validation path — non-object roots must validate, not crash (#1308) | ||
| const outputObj = normalizeObjectSchema(tool.outputSchema); |
There was a problem hiding this comment.
Should the !result.structuredContent check above also take this fallback into account? With an .optional() or .nullish() root, a tool that returns only content still gets has an output schema but no structured content was provided before this parse runs (I tried a quick local test at this commit with both roots), so the optional part of the schema never applies. If that's intended, since the spec expects structured content whenever an output schema is set, it might be worth a sentence in the changeset so people don't expect .optional() to make structuredContent optional. Otherwise a case where the handler omits structuredContent could go into the new test.
Fixes #1308 (with the same reproduction as my comment; @lawrence3699 offered a v1.x fix in April — nothing was opened, so here one is, with a nod to that comment).
Root cause
validateToolOutputnormalizes before parsing:For a non-object root (
.optional(),.nullable(),.nullish(),z.union(...)),normalizeObjectSchemareturnsundefined, andsafeParseAsynccrashes readingundefined._zod— every call to the tool fails. The input path already handles this (inputObj ?? tool.inputSchema,onCallTool); the output path never got the fallback.The same registration also silently omits the schema from
tools/list(the listing only serializes when normalization succeeds), which disables client-side success validation:cacheToolMetadataonly builds validators for tools whose listing carriedoutputSchema.Fix
src/server/mcp.ts(validateToolOutput)outputObj ?? tool.outputSchema— mirrors the input path; all four root shapes now validatesrc/shared/outputSchemaWarning.tsvalidateAndWarnToolNamepattern), so the listing omission is visible instead of silenttest/server/mcp.test.tsWhy the listing cannot convert instead: the wire schema requires
type: "object"at theoutputSchemaroot (ToolSchema, per the MCP specification), and clients parse listings against it — emitting theanyOfa zod union converts to would make spec-valid clients rejecttools/listitself. So the omission is unavoidable; the warning plus the pinned test make it non-silent and documented. Servers that want advertised schemas are pointed at the wrapper shape (z.objectwith an optionalerrorproperty — the envelope pattern discussed in #2748).Verification
Node 22.15.0, against
v1.x@3b73021a.npx vitest run test/server/mcp.test.ts→ 248 passed (246 before, 2 new: one test × the zod matrix)npx vitest run→ 2994/2995. The one failure istest/e2e/scenarios/protocol.test.ts > protocol:timeout:max-total, which reproduces identically on a pristinev1.xcheckout with this branch stashed (fake-timer flake, also noted on docs(client): say what v1.x actually does with structuredContent on error results #2836) — pre-existing and unrelated.??fallback fails exactly the new test (both zod versions) and leaves the rest green.npm run lint→ clean (the warning lives insrc/shared/following thetoolNameValidationprecedent, sinceno-consoleis enforced undersrc/server/);npm run build→ clean.Changeset: patch.